Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

test/models/brex_entry/processor_test.rb (1)

25-34: ⚡ Quick win

Assert the merchant payload is actually sanitized.

This fixture already includes card_metadata.pan, but the test never proves that the saved extra["brex"]["merchant"] excludes it. Given the security-sensitive sanitization work in this PR, I'd lock that in here.

🧪 Suggested assertion

     assert_equal BigDecimal("12.34"), entry.amount
     assert_equal "USD", entry.currency
     assert_equal "brex", entry.source
     assert_equal Date.new(2026, 1, 2), entry.date
     assert_equal "STAPLES", entry.transaction.merchant.name
     assert_equal "card_1", entry.transaction.extra.dig("brex", "card_id")
+    refute_includes entry.transaction.extra.dig("brex", "merchant").to_s, "test-pan-placeholder"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/models/brex_entry/processor_test.rb` around lines 25 - 34, The test
"imports card purchase with Brex signed amount preserved" doesn’t assert that
merchant data in the saved payload is sanitized; update this test (involving
BrexEntry::Processor and the resulting entry variable) to explicitly check
entry.transaction.extra.dig("brex","merchant") does not include
card_metadata.pan (or any PAN field) and that expected merchant fields remain
present, ensuring the saved extra["brex"]["merchant"] has sensitive PAN removed
while other merchant attributes are preserved.

app/models/brex_entry/processor.rb (1)

112-116: ⚡ Quick win

Add provider-specific invalid-currency logging.

parse_currency can reject provider data here, but this processor never overrides log_invalid_currency, so those warnings won't include the Brex transaction/account context that the other provider processors log.

🪵 Suggested change

   def currency
     parse_currency(BrexAccount.currency_code_from_money(data[:amount])) ||
       parse_currency(brex_account.currency) ||
       "USD"
   end
+
+  def log_invalid_currency(currency_code)
+    Rails.logger.warn(
+      "BrexEntry::Processor - Invalid currency '#{currency_code}' for transaction #{data[:id]} on brex_account #{brex_account.id}"
+    )
+  end

Based on learnings, standardize invalid-currency logging by overriding `log_invalid_currency` in each provider processor with provider-specific context.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_entry/processor.rb` around lines 112 - 116, The currency
resolution in BrexEntry::Processor (method currency) can trigger parse_currency
rejections but this class never overrides log_invalid_currency, so those
warnings lack Brex-specific context; add an override of log_invalid_currency in
the BrexEntry::Processor class that formats and emits provider-specific details
(e.g., include data[:id], data[:amount], BrexAccount.currency_from_money or
brex_account.id/account identifier) and then calls or delegates to the shared
logger behavior so invalid-currency warnings include Brex transaction/account
context; reference the currency method, parse_currency call,
BrexAccount.currency_code_from_money, and brex_account.currency when building
the message.

app/views/brex_items/_brex_item.html.erb (1)

87-102: ⚡ Quick win

Precompute sync/account data before rendering this partial.

This block does query/fallback work in ERB (syncs.ordered.first, repeated association-backed checks/counts), which makes the partial responsible for data access and can add extra queries per Brex item on the accounts index. Pass the prepared stats/counts in, or wrap the lookup in a helper/component method so the template only renders values.

As per coding guidelines "Avoid heavy logic in ERB views; prefer helpers and components" and "Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/brex_items/_brex_item.html.erb` around lines 87 - 102, The partial
is performing data lookups (brex_item.syncs.ordered.first&.sync_stats and
association-backed counts) which may trigger extra queries; move this logic out
of the ERB by precomputing and passing in the values (stats, unlinked_count,
linked_count, total_count, institutions_count) from the controller/presenter or
by adding a helper method (e.g., fetch_brex_stats(brex_item,
brex_sync_stats_map) or BrexItemPresenter methods) and update the call site that
renders this partial to provide those locals instead of computing them inside
the template; ensure ProviderSyncSummary still receives the stats param and
replace uses of `@brex_sync_stats_map` and brex_item.*_accounts_count in the ERB
with the passed-in locals.

app/views/brex_items/select_accounts.html.erb (1)

11-12: ⚡ Quick win

Replace raw <form> with form_with to avoid manual CSRF token management.

The manual hidden_field_tag :authenticity_token, form_authenticity_token works today, but it's easy to accidentally drop in a refactor. form_with handles the CSRF token automatically and is idiomatic Rails.

♻️ Proposed refactor

-        <form action="<%= link_accounts_brex_items_path %>" method="post" class="space-y-4" data-turbo-frame="_top">
-          <%= hidden_field_tag :authenticity_token, form_authenticity_token %>
+        <%= form_with url: link_accounts_brex_items_path, method: :post, class: "space-y-4", data: { turbo_frame: "_top" } do |_form| %>

-        </form>
+        <% end %>

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/brex_items/select_accounts.html.erb` around lines 11 - 12, Replace
the raw <form> and manual CSRF hidden_field_tag with Rails' form_with helper:
call form_with url: link_accounts_brex_items_path, method: :post and pass the
html options (class: "space-y-4", data: { turbo_frame: "_top" }) so the form
uses the same action and attributes; remove the hidden_field_tag
:authenticity_token, form_authenticity_token because form_with will
automatically include the CSRF token. Ensure you update the closing tag/ERB
block that currently wraps the form contents to use form_with's block form.

app/models/brex_item/account_flow.rb (5)

622-627: 💤 Low value

Hard-coded subtype strings — surface them as constants near the consuming model.

"credit_card" and "checking" are subtype identifiers tied to CreditCard::SUBTYPES and Depository::SUBTYPES respectively. Hard-coding them here makes a future rename in those constants silently break Brex setup defaults. Consider referencing a constant on the accountable (e.g. CreditCard::DEFAULT_SUBTYPE/Depository::DEFAULT_SUBTYPE) so renames fail loudly at boot.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 622 - 627, The method
selected_subtype_for in account_flow.rb currently returns hard-coded strings
"credit_card" and "checking"; replace those literals with constants on the
related models (e.g. CreditCard::DEFAULT_SUBTYPE and
Depository::DEFAULT_SUBTYPE) so renames fail loudly. Update selected_subtype_for
to reference those constants when selected_type == "CreditCard" and
selected_type == "Depository", and add/ensure the constants (DEFAULT_SUBTYPE or
similarly named) exist on CreditCard and Depository to point to the current
default values.

---

88-185: ⚖️ Poor tradeoff

Significant duplication between select_accounts_result and select_existing_account_result.

The two methods share substantial structure: both call selection_failure_result, both filter accounts (with slightly different empty messages), both build SelectionResult for each branch, and both have nearly identical rescue chains for NoApiTokenError/Provider::Brex::BrexError/StandardError differing only in the i18n scope. Extracting a private helper that takes a config hash (scope_key, accountable_type, success/empty messages) would remove ~70 lines of repetition and make future status additions a one-place change.

♻️ Sketch

def selection_result_for(scope:, accountable_type:, empty_message_key:)
  return selection_failure_result(scope, accountable_type: accountable_type) unless selected?

  accounts = filter_accounts(unlinked_available_accounts, accountable_type)
  return selection_result(:empty, accountable_type, I18n.t("#{scope}.#{empty_message_key}")) if accounts.empty?

  selection_result(:success, accountable_type, nil, available_accounts: accounts)
rescue NoApiTokenError
  selection_result(:no_api_token, accountable_type, I18n.t("#{scope}.no_api_token"))
rescue Provider::Brex::BrexError => e
  Rails.logger.error("Brex API error in #{scope}: #{e.message}")
  selection_result(:api_error, accountable_type, e.message)
rescue StandardError => e
  Rails.logger.error("Unexpected error in #{scope}: #{e.class}: #{e.message}")
  selection_result(:unexpected_error, accountable_type, I18n.t("#{scope}.unexpected_error"))
end

As per coding guidelines: avoid violations of DRY in changed code.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 88 - 185, The two methods
select_accounts_result and select_existing_account_result are nearly identical;
extract a private helper (e.g., selection_result_for or
selection_result_with_scope) that accepts parameters like scope_key
(string/symbol for i18n keys and logging), accountable_type, and an
empty_message_key, centralizes the filter_accounts/unlinked_available_accounts
logic, builds SelectionResult via a small selection_result(...) helper, and
consolidates the rescue handlers (NoApiTokenError, Provider::Brex::BrexError,
StandardError) so both original methods simply call this helper with their scope
and message keys; update select_accounts_result and
select_existing_account_result to delegate to that helper and remove the
duplicated blocks.

---

286-301: 💤 Low value

import_accounts_from_api_if_needed will not refresh accounts after the initial import.

The early return return nil unless brex_item.brex_accounts.empty? means once any account has been persisted, this method becomes a no-op — even if the user added new accounts on the Brex side and reopened the setup modal. As a result, BrexItems::AccountSetupsController#setup_accounts never picks up newly-available accounts in the setup UI; users would need to wait for the next background sync. If that's intentional, a comment would help; otherwise consider re-fetching when the cache is also empty/stale, or distinguish "no accounts ever" from "no new accounts since last sync".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 286 - 301, The current
guard in import_accounts_from_api_if_needed (return nil unless
brex_item.brex_accounts.empty?) prevents any subsequent refresh once a single
account exists; change the logic to always fetch available_accounts (after still
raising NoApiTokenError if credentials aren't configured), return nil only if
fetched accounts are empty, then upsert only missing or updated accounts by
comparing fetched account ids to existing brex_item.brex_accounts (e.g.,
existing_ids = brex_item.brex_accounts.pluck(:external_id)) and calling
upsert_brex_account! for accounts whose id is not present or whose attributes
differ; keep the NoApiTokenError and preserve final nil return.

---

67-86: 💤 Low value

preload_payload ignores cached value's emptiness for write-skipping but recomputes regardless on cache miss.

Two small concerns:

1. The cached array short-circuit at line 71-72 treats any cached value (including []) as "valid cache" via unless cached_accounts.nil?, which is correct, but the boolean has_accounts returned for cached: true is computed from the cached array. Make sure the writer in accounts (private) and any cache invalidation paths never store a stale [] as if it were a real result; otherwise users will see "no accounts" until the 5-minute TTL expires.
2. preload_payload writes the cache (line 75) and then accounts (line 570) re-reads + writes again. They share cache_key so it's fine in steady state, but a request that calls preload_payload followed by select_accounts_result will perform two cache reads where one would do.

Optional cleanup: have preload_payload delegate to accounts rather than duplicate the read/fetch/write pattern.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 67 - 86, preload_payload
currently duplicates cache read/fetch/write logic and can cause double cache
reads and stale empty-array behavior; change preload_payload to delegate to the
existing private accounts method (call accounts or accounts(force: false) as
appropriate) instead of re-implementing read/fetch/write, so it reuses the
single authoritative cache_key/CACHE_TTL behavior and avoids a second
read/write; also ensure the accounts writer only writes results when the fetch
succeeds (so it does not persist a stale []), and keep cached presence checks
driven by Rails.cache.exist?(cache_key) or the accounts method return value
rather than testing for nil directly.

---

358-411: ⚖️ Poor tradeoff

complete_setup! holds transaction locks across multiple account creation iterations unnecessarily.

With skip_initial_sync: true on each Account.create_and_sync call, no background jobs are enqueued during account creation—only database operations (save, opening balance, account shares) execute within the transaction. However, the entire loop runs inside a single ActiveRecord::Base.transaction, meaning all created accounts remain locked for the duration of all iterations. If any account creation fails late in the loop, every prior account is rolled back.

Consider per-account transactions so a single bad account doesn't undo all prior successful setups, unless atomic-all-or-none behavior is strictly required for your use case.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 358 - 411, The current
complete_setup! wraps the entire account_types loop in one
ActiveRecord::Base.transaction which holds locks across all account creations;
change this to use a per-account transaction around each create path (wrap the
Account.create_and_sync and AccountProvider.create! for each brex_account in its
own transaction) so a failure creating one account only rolls back that account
instead of all prior ones, while leaving brex_item.sync_later and the final
SetupResult behavior unchanged; alternatively, if you truly need atomic
all-or-none semantics, document that and keep the outer transaction.

app/models/family/brex_connectable.rb (1)

1-29: 💤 Low value

LGTM — concern follows the existing *Connectable pattern.

Symmetrical with other provider concerns: has_many :brex_items, dependent: :destroy, create_*_item! followed by sync_later, and a credentials predicate. Triggering sync_later after create! keeps the object-creation contract clean.

One small note on has_brex_credentials?: brex_items.active.any?(&:credentials_configured?) loads all active items into memory and iterates in Ruby. For most families this is fine, but if credentials_configured? is expressible as a SQL-side condition (e.g. where.not(token: [nil, ""])), pushing it into a scope (brex_items.active.with_credentials.exists?) avoids the N-row materialization. Optional.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/family/brex_connectable.rb` around lines 1 - 29, The
has_brex_credentials? method currently materializes all active BrexItem records
and calls credentials_configured? in Ruby; instead add a DB-side scope (e.g. in
the BrexItem model define scope :with_credentials, -> { where.not(token: [nil,
""]) } or whichever SQL condition matches credentials_configured?) and change
Family::BrexConnectable#has_brex_credentials? to use
brex_items.active.with_credentials.exists? so the check runs in SQL and avoids
loading all rows.

app/models/brex_item/importer.rb (3)

164-179: 🏗️ Heavy lift

store_new_transactions rewrites the entire transactions JSONB on every page — verify with bounded sync windows.

upsert_brex_transactions_snapshot!(existing_transactions + new_transactions) rewrites the full payload on each sync. With Provider::Brex pagination caps (MAX_PAGES=250, DEFAULT_LIMIT=1000) raised in PR feedback, this can store hundreds of thousands of transactions in a single JSONB column, growing unbounded per account. Each subsequent sync deserializes/serializes the entire history, allocates a with_indifferent_access wrapper per existing entry for dedup, and writes the union back.

Two complementary improvements:

1. Bound the appended history to a rolling window (e.g., last N days or last K rows) so the JSONB doesn't grow unboundedly.
2. Pass sync.window_start_date (per jjmata's review feedback) into determine_sync_start_date so we don't re-fetch and re-merge the full history each time.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/importer.rb` around lines 164 - 179,
store_new_transactions currently reads the full
brex_account.raw_transactions_payload, dedups against all IDs, then calls
upsert_brex_transactions_snapshot! with the union—this rewrites an ever-growing
JSONB each page; instead, add a window_start_date parameter (pass
sync.window_start_date into determine_sync_start_date and thread it into
store_new_transactions) and only keep/merge transactions within that rolling
window (e.g., last N days or last K rows) before deduping and upserting; prune
existing_transactions to the window, select new_transactions whose timestamps
are >= window_start_date, and call upsert_brex_transactions_snapshot! with the
pruned union so we no longer deserialize/serialize the entire history on every
sync.

---

56-60: 💤 Low value

Snapshot persistence failure is silently swallowed.

store_item_snapshot rescues all errors and only logs. The import continues to call import_accounts/import_transactions even though the institution/raw_payload snapshot wasn't stored — meaning views that rely on raw_payload/raw_institution_payload may be stale or empty without any visible signal to the user (brex_item.status stays whatever it was). Consider re-raising for non-transient failures, or at least reporting via Sentry like BrexAccount::Processor does.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/importer.rb` around lines 56 - 60, The current
store_item_snapshot method swallows all exceptions from
brex_item.upsert_brex_snapshot! which allows the import to continue with missing
raw_payload; change store_item_snapshot (and its rescue) so that on error you
both report the exception to the error tracker (e.g. Sentry.capture_exception or
the same mechanism used by BrexAccount::Processor) including context (brex_item
id, accounts_data) and then re-raise the exception (or set a durable failure
state on brex_item such as updating brex_item.status to a snapshot failure state
before re-raising) so the overall import halts and the failure is visible.

---

23-23: 💤 Low value

Status reset never marks healthy items back to :good after partial failure recovery — but conversely, a non-credentials transient error keeps the item in whatever state it was previously in.

brex_item.update!(status: :good) only fires when the entire import has zero failures. After a transient API error caused mark_requires_update_if_credentials_error to set :requires_update once (on a 401/403), a subsequent successful sync will move it back to :good. That's correct. However, if the prior failure was a non-credentials error that didn't change status, and the current sync has any failures (e.g., one transient transaction failure), the item may stay in a stale status indefinitely. Consider whether the success/health flag should reflect the latest sync regardless of prior state, or split status into "credentials valid" vs. "last sync ok".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/importer.rb` at line 23, The current logic lets prior
non-credentials failures leave brex_item in a stale non-:good state; ensure the
item’s status reflects the latest sync by unconditionally setting status to
:good when the current import had zero account and zero transaction failures
(i.e., when account_result[:accounts_failed].zero? &&
transaction_result[:transactions_failed].zero?), and make sure
mark_requires_update_if_credentials_error only sets :requires_update on 401/403
without blocking later status updates — move or adjust the
brex_item.update!(status: :good) call and any early returns so the status reset
runs based on the current results regardless of previous state.

app/models/brex_account/transactions/processor.rb (2)

44-51: 💤 Low value

Trim full backtrace logging to first N frames for consistency.

Rails.logger.error Array(e.backtrace).join("\n") writes the full backtrace (often dozens of frames) for every failed transaction. With a malformed payload, this can flood logs. Other Brex code (e.g. BrexAccount::Processor#process_transactions and BrexItem::AccountFlow#complete_setup_result) uses Array(e.backtrace).first(10). Consider matching that limit here.

♻️ Proposed change

-        Rails.logger.error Array(e.backtrace).join("\n")
+        Rails.logger.error Array(e.backtrace).first(10).join("\n")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_account/transactions/processor.rb` around lines 44 - 51, The
rescue block currently logs the full exception backtrace via Rails.logger.error
Array(e.backtrace).join("\n"), which can flood logs; update the error logging in
the rescue (the code around failed_count, transaction_id_for(transaction_data),
and errors << ...) to instead log only the first N frames (match other Brex code
by using Array(e.backtrace).first(10)) and join those frames for the
Rails.logger.error call so the rest of the rescue behavior (incrementing
failed_count and appending to errors) remains unchanged.

---

30-33: 💤 Low value

Consider a more specific signal for "skipped" vs. "failed" transactions.

A nil return from BrexEntry::Processor is currently bucketed into failed_count with the hard-coded reason "No linked account". If the processor ever returns nil for a different valid skip reason, both the count and the error message would become misleading. Returning a dedicated sentinel (e.g. a symbol :skipped) or splitting failed_count from skipped_count would make the result safer for future evolution.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_account/transactions/processor.rb` around lines 30 - 33, The
current logic treats any nil result from BrexEntry::Processor as a failure with
message "No linked account"; change this to a clear distinction by having
BrexEntry::Processor return a sentinel (e.g. :skipped) for intentional skips and
update the caller (the code handling result in processor.rb) to branch: if
result == :skipped increment a new skipped_count and record a skip reason (or
omit error), if result.nil? keep incrementing failed_count and record the real
failure message (or transaction_id_for(transaction_data)); update any aggregated
reporting to include skipped_count. Ensure you reference BrexEntry::Processor
and the result handling block in processor.rb when making these changes.

db/migrate/20260505010000_create_brex_items_and_accounts.rb (1)

32-57: ⚡ Quick win

Consider tightening currency null constraint at the DB layer.

BrexAccount validates currency presence in the model, but the column is nullable here. As per coding guidelines, simple validations (null checks, unique indexes) should be enforced in the database schema for PostgreSQL. Aligning the schema with the model invariant prevents bypass through direct SQL or future code paths.

♻️ Proposed change

-      t.string :currency
+      t.string :currency, null: false

You may want to also add a default (e.g. "USD") to make the migration safe to run on existing data, and align with BrexAccount.currency_code_from_money defaults used by the importer.

As per coding guidelines: "Use simple database validations (null checks, unique indexes) in database schema".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@db/migrate/20260505010000_create_brex_items_and_accounts.rb` around lines 32
- 57, The brex_accounts table currently allows NULL for the currency column but
the BrexAccount model requires presence; update the migration to enforce this at
DB level by changing the currency column to null: false and add a sensible
default (e.g. "USD") to make the migration safe for existing rows (use t.string
:currency, null: false, default: "USD"); ensure this aligns with
BrexAccount.currency_code_from_money and any importer logic so defaults are
consistent.

app/models/brex_account/processor.rb (1)

38-41: 💤 Low value

Consider raising or alerting on persistent currency parse failures rather than silently defaulting to USD.

Defaulting to "USD" when parse_currency fails keeps sync from breaking, but the warn log alone is easy to miss. If a non-USD account is silently converted to USD, downstream balance reporting will be subtly wrong. Consider a Sentry breadcrumb/capture_message here so it surfaces in monitoring without aborting the sync.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_account/processor.rb` around lines 38 - 41, In
BrexAccount::Processor, inside the branch where currency is nil after parse (the
block referencing currency and brex_account.id), add monitoring so failures are
visible: call Sentry.capture_message (or Sentry.add_breadcrumb +
capture_message) with a clear message like "BrexAccount currency parse failed",
include context/tags for brex_account.id and the raw brex_account.currency
value, and set level to warning/error; keep the current fallback to "USD" so
sync continues but ensure the capture includes useful context for debugging.

app/controllers/brex_items/account_setups_controller.rb (1)

15-33: 💤 Low value

Consider strong params for account_types / account_subtypes.

params[:account_types] and params[:account_subtypes] are passed straight through as raw hashes. While BrexItem::AccountFlow#complete_setup! does whitelist selected_type against Provider::BrexAdapter.supported_account_types, treating hash params as ActionController::Parameters and explicitly permitting them keeps behavior consistent with the rest of the controller layer and avoids surprises if these params are used differently in future.

As per coding guidelines: "Implement strong parameters and CSRF protection throughout the application".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/controllers/brex_items/account_setups_controller.rb` around lines 15 -
33, The controller currently passes raw hashes from params[:account_types] and
params[:account_subtypes] into brex_account_flow.complete_setup_result; update
complete_account_setup to sanitize these by using strong params: build
sanitized_account_types = params.fetch(:account_types, {}).permit(allowed_types)
and sanitized_account_subtypes = params.fetch(:account_subtypes,
{}).permit(allowed_subtypes), where allowed_types =
Provider::BrexAdapter.supported_account_types.map(&:to_s) (and use the
equivalent supported list for subtypes or an explicit whitelist), then pass
those sanitized vars into brex_account_flow.complete_setup_result instead of the
raw params to ensure only expected keys are forwarded.

app/models/brex_item.rb (1)

33-44: ⚡ Quick win

Avoid raising bare StandardError.

raise StandardError.new("Brex provider is not configured") forces every caller to rescue the universal base class to catch this specific condition, which collides with unrelated failures. Define a domain-specific error (or reuse Provider::Brex::BrexError) so the importer/sync layers can branch on it deterministically.

♻️ Proposed change

+  class ProviderNotConfiguredError < StandardError; end
+
   def import_latest_brex_data(sync_start_date: nil)
     provider = brex_provider
     unless provider
       Rails.logger.error "BrexItem #{id} - Cannot import: provider is not configured"
-      raise StandardError.new("Brex provider is not configured")
+      raise ProviderNotConfiguredError, "Brex provider is not configured"
     end

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item.rb` around lines 33 - 44, The method
import_latest_brex_data currently raises a bare StandardError when the provider
is missing; change this to raise a domain-specific error (e.g.,
Provider::Brex::BrexError or a new BrexProviderNotConfiguredError) so callers
can rescue the specific condition; keep the existing Rails.logger.error line,
and raise the chosen domain error from import_latest_brex_data (and ensure any
existing rescue blocks that expect provider-specific errors use the new error
type, e.g., in BrexItem::Importer and calling sync/import layers).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/models/brex_item/account_flow.rb (2)

188-202: 💤 Low value

Consider wrapping link_existing_account! in a transaction for symmetry with link_new_accounts!.

link_new_accounts! (line 142) wraps upsert + AccountProvider creation in ActiveRecord::Base.transaction, but link_existing_account! does not. If AccountProvider.create! (line 198) fails, the just-upserted brex_account (line 195) is committed without a link. Retries are idempotent so functional impact is low, but a transaction would keep the two paths symmetric and avoid orphaned snapshots when a partial failure occurs.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 188 - 202, Wrap the upsert
+ provider creation in link_existing_account! in an
ActiveRecord::Base.transaction to match link_new_accounts!: call
upsert_brex_account! and AccountProvider.create! inside a transaction so a
failed create! rolls back the upsert, then call brex_item.sync_later after the
transaction and return brex_account; reference the existing methods
link_existing_account!, upsert_brex_account!, AccountProvider.create!, and
link_new_accounts! to mirror its behavior.

---

344-354: 💤 Low value

Method name suggests a query but performs the import.

import_accounts_error_message calls import_accounts_from_api_if_needed (a side-effecting operation that hits the Brex API and writes to the DB) and only returns an error message on failure. Callers reading this name on a controller would reasonably assume it's a pure error-formatting method. Consider a name like import_accounts_with_user_facing_error or splitting the import call from the rescue/i18n translation.

♻️ Possible rename

-  def import_accounts_error_message
+  def import_accounts_or_error_message
     import_accounts_from_api_if_needed
   rescue NoApiTokenError
     I18n.t("brex_items.setup_accounts.no_api_token")
   ...
   end

Adjust callers accordingly.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 344 - 354, The method
import_accounts_error_message performs a side-effecting import by calling
import_accounts_from_api_if_needed but its name implies a pure query; rename it
to import_accounts_with_user_facing_error (or split into two: keep
import_accounts_from_api_if_needed for the side-effect and create a pure
import_accounts_error_message that only formats/errors) and update all callers
to either call the new importer method then call the pure error-formatting
helper, or call the new combined import_accounts_with_user_facing_error; ensure
you update references to import_accounts_error_message and
import_accounts_from_api_if_needed accordingly and preserve the same rescue/I18n
behavior during the rename/split.

app/views/accounts/index.html.erb (1)

52-62: 💤 Low value

Inconsistent rendering pattern — Brex uses explicit locals while every other provider uses implicit collection rendering.

Every other provider in this file uses <%= render @x_items.sort_by(&:created_at) %> and lets the partial pick up shared instance variables itself. The Brex block instead computes locals in the view via brex_item_render_locals(...). Consider moving the helper call inside app/views/brex_items/_brex_item.html.erb (or the helper invocation into the partial's first line) so the index can use the same one-liner pattern as Plaid/SimpleFIN/Mercury.

♻️ Suggested change

-    <% if `@brex_items.any`? %>
-      <% `@brex_items.sort_by`(&:created_at).each do |brex_item| %>
-        <%= render partial: "brex_items/brex_item",
-                   locals: brex_item_render_locals(
-                     brex_item,
-                     sync_stats_map: `@brex_sync_stats_map`,
-                     account_counts_map: `@brex_account_counts_map`,
-                     institutions_count_map: `@brex_institutions_count_map`
-                   ) %>
-      <% end %>
-    <% end %>
+    <% if `@brex_items.any`? %>
+      <%= render `@brex_items.sort_by`(&:created_at) %>
+    <% end %>

Then have _brex_item.html.erb open with <% locals = brex_item_render_locals(brex_item, sync_stats_map: @brex_sync_stats_map, account_counts_map: @brex_account_counts_map, institutions_count_map: @brex_institutions_count_map) %> (or merge directly into the existing locals).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/accounts/index.html.erb` around lines 52 - 62, Replace the explicit
per-item locals in the index view by rendering the Brex collection like the
others (use render `@brex_items.sort_by`(&:created_at)), and move the
brex_item_render_locals(...) call into the Brex partial so it computes needed
locals for each brex_item; specifically, update
app/views/brex_items/_brex_item.html.erb to begin by calling
brex_item_render_locals(brex_item, sync_stats_map: `@brex_sync_stats_map`,
account_counts_map: `@brex_account_counts_map`, institutions_count_map:
`@brex_institutions_count_map`) (or merge its values into the partial’s existing
locals) and adjust the partial to use those locals so the index block can be a
one-liner like the other providers.

test/models/brex_account/transactions/processor_test.rb (1)

24-34: 💤 Low value

Coverage is narrow — consider asserting the success/imported path too.

This file only validates the "no linked account" skip branch. The processor surface (total/imported/skipped/failed/errors/skipped_transactions) is broad enough that a happy-path test (linked account + imported entry) and at least one failure-path test (e.g., processor raising for one entry while others succeed) would lock in the aggregation contract. Not a blocker; flagging as a coverage gap rather than a defect.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/models/brex_account/transactions/processor_test.rb` around lines 24 -
34, Add two more tests for BrexAccount::Transactions::Processor#process: one
"happy-path" that sets up a linked account and verifies result[:success] true,
result[:total] increments, result[:imported] increments, result[:skipped] and
result[:failed] are zero, and skipped_transactions/errors are empty; and one
"partial-failure" test that rigs one entry to raise (or simulate a failed
import) while others succeed and then asserts aggregated counts (total,
imported, skipped, failed), presence of an error in result[:errors], and that
the failed entry is represented appropriately in result[:failed] or
result[:errors]; use the same result keys
(total/imported/skipped/failed/errors/skipped_transactions) and
BrexAccount::Transactions::Processor.new(`@brex_account`).process to exercise the
aggregation contract.

app/views/brex_items/_brex_item.html.erb (1)

22-24: 💤 Low value

Use semantic markup consistent with surrounding tag.p.

Line 21 uses tag.p for the item name and line 23 uses a raw <p> for the deletion-in-progress label inside the same flex row. This works but is inconsistent with the helper-based tag usage elsewhere in the partial. Using tag.p keeps the file consistent and benefits from automatic escaping/CSP-friendliness.

♻️ Suggested tweak

-            <% if brex_item.scheduled_for_deletion? %>
-              <p class="text-destructive text-sm animate-pulse"><%= t(".deletion_in_progress") %></p>
-            <% end %>
+            <% if brex_item.scheduled_for_deletion? %>
+              <%= tag.p t(".deletion_in_progress"), class: "text-destructive text-sm animate-pulse" %>
+            <% end %>

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/brex_items/_brex_item.html.erb` around lines 22 - 24, Replace the
raw HTML paragraph used for the deletion label with the Rails tag helper to
match the surrounding markup: in the conditional that checks
brex_item.scheduled_for_deletion? swap the literal <p class="..."> containing
t(".deletion_in_progress") for tag.p with the same class list (e.g.,
"text-destructive text-sm animate-pulse") so it uses the helper-based rendering
and automatic escaping consistent with the other tag.p usage in this partial.

app/controllers/brex_items/account_flows_controller.rb (2)

30-55: ⚡ Quick win

Handle missing account ids without surfacing a 404 to the user.

Current.family.accounts.find(params[:account_id]) on lines 33 and 48 will raise ActiveRecord::RecordNotFound if the id is for an account from another family or simply doesn't exist. Unless ApplicationController already rescues this for HTML/Turbo flows (and renders a friendly redirect), a malformed/expired link will produce a 404 inside a Turbo modal flow rather than a redirect_to accounts_path with a flash. Recommend either using find_by(id: …) with an explicit redirect (mirroring the params[:account_id].blank? early return) or adding a rescue_from ActiveRecord::RecordNotFound for this controller.

♻️ Suggested change

   def select_existing_account
     return redirect_to accounts_path, alert: t("brex_items.select_existing_account.no_account_specified") if params[:account_id].blank?

-    `@account` = Current.family.accounts.find(params[:account_id])
+    `@account` = Current.family.accounts.find_by(id: params[:account_id])
+    return redirect_to accounts_path, alert: t("brex_items.select_existing_account.no_account_specified") if `@account.nil`?
     result = brex_account_flow.select_existing_account_result(account: `@account`)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/controllers/brex_items/account_flows_controller.rb` around lines 30 - 55,
The controller currently calls Current.family.accounts.find(params[:account_id])
in select_existing_account and link_existing_account which will raise
ActiveRecord::RecordNotFound for invalid/other-family IDs; change both to use
Current.family.accounts.find_by(id: params[:account_id]) and if that returns nil
perform the same redirect used for blank params (redirect_to accounts_path,
alert: t("brex_items.select_existing_account.no_account_specified") for
select_existing_account and the analogous
t("brex_items.link_existing_account.no_account_specified") or reuse the same
message for link_existing_account), so that
brex_account_flow.select_existing_account_result and
brex_account_flow.link_existing_account_result are only called with a valid
account and no 404 is surfaced in Turbo/modal flows.

---

98-116: 💤 Low value

Consider rejecting percent-encoded slash/backslash sequences in safe_return_to_path for defense in depth.

The current implementation correctly blocks //evil, /\evil, control chars, whitespace, and absolute URLs. However, percent-encoded variants such as /%2fevil.example/path and /%5cevil.example/path will pass: URI.parse keeps them as a path with no scheme/host, and the second character check sees %, not / or \. While most browsers don't decode these in Location headers, intermediaries (proxies, link rewriters) sometimes do, and adding the check is essentially free.

🛡️ Suggested hardening

       second_character = return_to[1]
       return nil if second_character.blank?
       return nil if second_character == "/" || second_character == "\\"
       return nil if second_character.match?(/[[:space:][:cntrl:]]/)
+      return nil if return_to.match?(/\A\/(?:%2f|%5c)/i)

       uri = URI.parse(return_to)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/controllers/brex_items/account_flows_controller.rb` around lines 98 -
116, The safe_return_to_path method should reject percent-encoded
slash/backslash sequences to harden against proxies that decode them; after
computing return_to (from params[:return_to].to_s.strip) and before URI.parse,
check the bytes at positions 1..3 (the percent-encoded sequence immediately
after the leading "/") and return nil if they match %2f or %5c
(case-insensitive) or otherwise decode to "/" or "\"; keep the existing
second_character checks and the URI.parse host/scheme checks but add this
explicit percent-encoding rejection so strings like "/%2fevil" or "/%5cevil" are
rejected by safe_return_to_path.

test/controllers/brex_items_controller_test.rb (1)

313-347: 💤 Low value

Verify the unsupported-type branch covers the actual production filter.

The test asserts that passing account_types[unsupported_brex_account.id] = "Investment" results in unsupported_brex_account.account_provider being nil after complete_account_setup. That's a meaningful assertion, but it only confirms a side-effect, not which guard rejected it (unsupported accountable_type, unsupported account_kind, missing brex_account lookup, etc.). Consider also asserting that flash[:notice] (or an analogous message) reports a partial setup, or that the controller exposes a count of skipped/unsupported entries — otherwise a future refactor that silently swallows valid Investment requests would still pass this test.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/controllers/brex_items_controller_test.rb` around lines 313 - 347, The
test currently only checks the side-effect that
unsupported_brex_account.account_provider is nil after calling
complete_account_setup_brex_item_url; update the test to also assert the
controller reported the skipped/unsupported entry (e.g. assert_match
/skipp|unsupported|partial/i, flash[:notice] || flash[:alert]) so you verify the
actual guard that rejected the "Investment" account type is surfaced to the
user; if the controller doesn't set such a flash, modify the
complete_account_setup action to set flash[:notice] with a message or expose a
skipped_count (e.g. `@skipped_count`) and assert that value in the test instead
(referencing complete_account_setup_brex_item_url, unsupported_brex_account, and
AccountProvider.count).

app/views/brex_items/select_accounts.html.erb (1)

19-43: 💤 Low value

Provide an explicit submit-disabled state when no selectable accounts exist.

If every entry in @available_accounts has a blank name, every checkbox is disabled, but the "Link Accounts" submit button stays enabled. Submitting then posts account_ids=[] to link_accounts_brex_items_path, which the controller has to special-case. Consider disabling the submit button (and/or rendering an empty-state message) when @available_accounts.none? { |a| !brex_account_display(a).blank_name? } so the UI doesn't invite a no-op submission.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/brex_items/select_accounts.html.erb` around lines 19 - 43, The form
should disable the submit action when there are no selectable accounts: compute
a boolean like has_selectable = `@available_accounts.any`? { |a|
!brex_account_display(a).blank_name? } and use it to disable the "Link Accounts"
submit button (e.g. add disabled: true and a disabled visual style/class) and/or
render an explicit empty-state message; update any view helper usage around
brex_account_display and check_box_tag to rely on has_selectable so the submit
cannot be clicked when all checkboxes are disabled.

app/controllers/brex_items/account_setups_controller.rb (1)

1-3: 💤 Low value

Run require_admin! before set_brex_item.

set_brex_item triggers a database lookup using the request param before the admin check has run. Reordering the callbacks ensures non-admins are rejected without any DB access and avoids leaking error states (e.g., RecordNotFound) that could differentiate between "missing item" and "not allowed" for non-admin callers.

♻️ Proposed reorder

-  before_action :set_brex_item
-  before_action :require_admin!
+  before_action :require_admin!
+  before_action :set_brex_item

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/controllers/brex_items/account_setups_controller.rb` around lines 1 - 3,
The before_action ordering in BrexItems::AccountSetupsController calls
set_brex_item before require_admin!, causing a DB lookup (and potential
RecordNotFound) before authorization; swap the callbacks so require_admin! runs
first (ensure the controller uses before_action :require_admin! followed by
before_action :set_brex_item) to reject non-admins without accessing the
database or leaking error details.

app/models/brex_account.rb (3)

161-167: 💤 Low value

Replace assign_attributes + save! with update!.

There's no intermediate logic between assignment and save, so update! is more idiomatic and equivalent.

♻️ Proposed simplification

 def upsert_brex_transactions_snapshot!(transactions_snapshot)
-  assign_attributes(
-    raw_transactions_payload: self.class.sanitize_payload(transactions_snapshot)
-  )
-
-  save!
+  update!(raw_transactions_payload: self.class.sanitize_payload(transactions_snapshot))
 end

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_account.rb` around lines 161 - 167, The method
upsert_brex_transactions_snapshot!(transactions_snapshot) currently calls
assign_attributes(...) followed by save!; replace that pair with a single call
to update! to be idiomatic and atomic. Specifically, update the method to call
update!(raw_transactions_payload:
self.class.sanitize_payload(transactions_snapshot)) instead of
assign_attributes(...) and save! so the sanitized payload is persisted in one
step.

---

64-77: ⚡ Quick win

money_to_decimal rescue path can re-raise ArgumentError.

If BigDecimal(amount.to_s) raises ArgumentError (e.g., the API returns a non-numeric amount), the rescue branch executes BigDecimal(payload[:amount].to_s) again on the same value, which will raise the same exception and propagate out of the method. Either (a) hard-default to BigDecimal("0") in the rescue or (b) catch the recurrence inside the rescue.

🛡️ Suggested fix

 rescue Money::Currency::UnknownCurrencyError, ArgumentError
   Rails.logger.warn("Invalid Brex money payload #{money_payload.inspect}, defaulting conversion to USD")
-  BigDecimal(payload[:amount].to_s) / BigDecimal(Money::Currency.new("USD").minor_unit_conversion.to_s)
+  begin
+    BigDecimal(payload[:amount].to_s) / BigDecimal(Money::Currency.new("USD").minor_unit_conversion.to_s)
+  rescue ArgumentError
+    nil
+  end
 end

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_account.rb` around lines 64 - 77, In money_to_decimal, the
rescue currently retries BigDecimal(payload[:amount].to_s) which can re-raise
the same ArgumentError; update the rescue block in the money_to_decimal method
to avoid propagating ArgumentError by wrapping the fallback BigDecimal(...) in
its own safe handler (or simply return BigDecimal("0") on failure), ensure you
still log the original payload (Rails.logger.warn(...)) and avoid calling
Money::Currency.new("USD").minor_unit_conversion on invalid amounts unless
guarded; reference the money_to_decimal method, the rescue clause,
BigDecimal(...) and Money::Currency usage when making the change.

---

13-15: 💤 Low value

linked_account duplicates account.

Both account and linked_account are declared has_one :through => :account_provider, source: :account, so they resolve to the same record. Pick one canonical name — using both invites confusion (e.g., calling linked_account and account triggers two distinct queries because each association caches independently).

♻️ Proposed simplification

   has_one :account_provider, as: :provider, dependent: :destroy
   has_one :account, through: :account_provider, source: :account
-  has_one :linked_account, through: :account_provider, source: :account

If linked_account is required for compatibility with existing call sites, alias it instead:

alias_method :linked_account, :account

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_account.rb` around lines 13 - 15, The BrexAccount model
defines both has_one :account and has_one :linked_account through
:account_provider with the same source (:account), causing duplicate, uncached
queries and confusion; fix by removing the redundant has_one :linked_account and
update callers to use :account, or if :linked_account must remain for
compatibility, replace the association with an alias (alias_method
:linked_account, :account) so both names refer to the same cached association;
locate the duplicate declarations in the BrexAccount class (has_one :account,
has_one :linked_account) and either delete the linked_account association or
convert it to an alias.

test/models/brex_item/importer_test.rb (1)

25-47: 💤 Low value

Tighten the start_date matcher to assert the exact value passed.

This test passes Date.new(2026, 1, 1) to the importer, so the matcher could assert start_date: Date.new(2026, 1, 1) instead of anything and still verify the same boundary contract that "uses explicit sync start date..." (line 132) covers. Using anything here weakens the assertion that the importer threads sync_start_date into get_cash_transactions for the discovery flow.

♻️ Suggested tightening

-    provider.expects(:get_cash_transactions).with("cash_1", start_date: anything).returns(
+    provider.expects(:get_cash_transactions).with("cash_1", start_date: Date.new(2026, 1, 1)).returns(

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/models/brex_item/importer_test.rb` around lines 25 - 47, The provider
expectation for get_cash_transactions uses a loose matcher (anything) for
start_date; tighten it to expect the exact sync start date passed to
BrexItem::Importer so the test verifies threading of sync_start_date through
discovery. Update the mock expectation on provider.get_cash_transactions to use
start_date: Date.new(2026, 1, 1) (matching the sync_start_date argument passed
to BrexItem::Importer.new) instead of anything; keep the rest of the expectation
and assertions unchanged.

app/models/provider/brex_adapter.rb (1)

115-131: 💤 Low value

Prefer metadata&.dig("name") over metadata&.[]("name").

The &.[] form works but is unidiomatic and harder to read. dig is the conventional Ruby way to safely access a nested hash key.

♻️ Proposed simplification

   def institution_name
     metadata = provider_account.institution_metadata
-
-    metadata&.[]("name") || item&.institution_name
+    metadata&.dig("name") || item&.institution_name
   end

   def institution_url
     metadata = provider_account.institution_metadata
-
-    metadata&.[]("url") || item&.institution_url
+    metadata&.dig("url") || item&.institution_url
   end

   def institution_color
     metadata = provider_account.institution_metadata
-
-    metadata&.[]("color") || item&.institution_color
+    metadata&.dig("color") || item&.institution_color
   end

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/provider/brex_adapter.rb` around lines 115 - 131, Replace the
unidiomatic safe-access calls using metadata&.[]("key") with
metadata&.dig("key") in the three accessors: institution_name, institution_url,
and institution_color; update the code that reads
provider_account.institution_metadata into metadata and return
metadata&.dig("name") || item&.institution_name, metadata&.dig("url") ||
item&.institution_url, and metadata&.dig("color") || item&.institution_color
respectively to make the hash lookups clearer and idiomatic.

app/models/brex_item.rb (2)

128-133: 💤 Low value

includes(:account) is unused in connected_institutions.

The block only reads acc.institution_metadata, which is a column on brex_accounts itself — there's no traversal of the account association inside the iteration. The eager-load is dead weight and will issue an extra query per call.

♻️ Proposed simplification

   def connected_institutions
-    brex_accounts.includes(:account)
-                  .where.not(institution_metadata: nil)
+    brex_accounts.where.not(institution_metadata: nil)
                   .map { |acc| acc.institution_metadata }
                   .uniq { |inst| inst["name"] || inst["institution_name"] }
   end

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item.rb` around lines 128 - 133, The includes(:account) in
the connected_institutions method is unnecessary because the code only reads
acc.institution_metadata (a column on brex_accounts) and does not traverse the
account association; remove the eager-load and instead iterate or pluck directly
from brex_accounts (e.g., use brex_accounts.where.not(institution_metadata:
nil).pluck(:institution_metadata).uniq { |inst| inst["name"] ||
inst["institution_name"] }) to avoid the extra query and de-duplicate by the
same uniqueness key.

---

85-91: 💤 Low value

Use update! instead of assign_attributes + save!.

There's no in-between logic, so update! is equivalent and more idiomatic. (Same pattern in BrexAccount#upsert_brex_transactions_snapshot!.)

♻️ Proposed simplification

 def upsert_brex_snapshot!(accounts_snapshot)
-  assign_attributes(
-    raw_payload: BrexAccount.sanitize_payload(accounts_snapshot)
-  )
-
-  save!
+  update!(raw_payload: BrexAccount.sanitize_payload(accounts_snapshot))
 end

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item.rb` around lines 85 - 91, Replace the assign_attributes
+ save! pattern in the method upsert_brex_snapshot! with a single update! call:
call update!(raw_payload: BrexAccount.sanitize_payload(accounts_snapshot))
inside the upsert_brex_snapshot! method; do the same refactor for the analogous
method BrexAccount#upsert_brex_transactions_snapshot! if present to keep the
pattern consistent and idiomatic.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/models/brex_item/account_flow.rb (1)

67-83: ⚡ Quick win

Replace the exist? + read pair with a single cache read.

Rails.cache.exist?(cache_key) followed by accounts (which calls Rails.cache.read internally on line 558) issues two cache lookups per preload_payload. Beyond the extra round-trip, there's a small TOCTOU window where the entry can expire between exist? and read: in that case accounts will re-fetch from the Brex API and rewrite the cache, but the payload still reports cached: true. Reading once and deriving cached from the result fixes both the inefficiency and the reporting drift.

♻️ Suggested refactor

   def preload_payload
     return selection_error_payload if !selected?
     return { success: false, error: "no_credentials", has_accounts: false } unless brex_item&.credentials_configured?

-    cached = Rails.cache.exist?(cache_key)
-    available_accounts = accounts
+    available_accounts = Rails.cache.read(cache_key)
+    cached = !available_accounts.nil?
+    unless cached
+      available_accounts = fetch_accounts
+      Rails.cache.write(cache_key, available_accounts, expires_in: CACHE_TTL)
+    end

     { success: true, has_accounts: available_accounts.any?, cached: cached }

Note: the existing accounts helper can stay for other call sites (unlinked_available_accounts, indexed_accounts) that don't care about cache-hit reporting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 67 - 83, Replace the two
cache lookups by doing a single Rails.cache.read(cache_key) inside
preload_payload: call cached_value = Rails.cache.read(cache_key), set cached =
!cached_value.nil? (or cached_value.present? if you want to treat empty arrays
as not-cached), then set available_accounts = cached_value || accounts (so
accounts is only called when the cache miss happens). Keep using the existing
accounts helper for the fetch/path that writes the cache; update the payload to
return cached based on the single read result.

app/views/brex_items/select_existing_account.html.erb (1)

24-24: 💤 Low value

Use a functional surface token instead of the raw bg-red-tint-5 color tint.

The code pairs functional tokens (border-destructive, text-destructive) with a raw color tint token (bg-red-tint-5), which violates the design system guideline: "Always prefer using functional tokens defined in sure-design-system.css rather than raw Tailwind utilities or arbitrary colors."

The design system currently lacks a destructive surface token. Replace bg-red-tint-5 with an existing functional surface token (e.g., bg-surface, bg-container) or add a new destructive surface utility (e.g., bg-destructive-subtle) to the design system for consistency.

This pattern appears in both select_existing_account.html.erb and select_accounts.html.erb at line 24, so a single fix covers both.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/views/brex_items/select_existing_account.html.erb` at line 24, Replace
the raw color token bg-red-tint-5 used in the label class conditional with a
functional surface token (e.g., bg-destructive-subtle or
bg-surface/bg-container) so the destructive state uses design-system semantics;
update the conditional in the label for account_display.blank_name? inside
select_existing_account.html.erb (and the identical spot in
select_accounts.html.erb) to use the chosen functional utility (e.g., change
"bg-red-tint-5" to "bg-destructive-subtle" or "bg-surface") and if your design
system lacks a destructive surface token, add a new utility
(bg-destructive-subtle) to sure-design-system.css and use that class in the
label conditional.

app/models/brex_item.rb (1)

86-88: 💤 Low value

Encryption inconsistency: BrexItem.raw_payload vs BrexAccount.raw_payload.

BrexAccount declares encrypts :raw_payload, but BrexItem.raw_payload is stored in plaintext. Both call BrexAccount.sanitize_payload to redact PII/secrets, so the immediate exposure is limited—but the asymmetry is surprising and a future change to sanitize_payload (or a missed sensitive key) loses the encryption-at-rest defense in depth on the parent record. Either encrypt both or document why the parent snapshot is intentionally plaintext.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item.rb` around lines 86 - 88, BrexItem currently stores
raw_payload plaintext in upsert_brex_snapshot! while BrexAccount declares
encrypts :raw_payload; to fix, add symmetric encryption on the parent model by
declaring encrypts :raw_payload in the BrexItem model (so
BrexItem.upsert_brex_snapshot! continues to call BrexAccount.sanitize_payload
but the stored snapshot is encrypted at rest), or if intentional, add an
explicit comment in BrexItem documenting why raw_payload is stored unencrypted
and reference BrexAccount.sanitize_payload and the threat model.

app/models/brex_item/syncer.rb (3)

50-50: ⚡ Quick win

N+1 query: current_account not preloaded.

current_account delegates to the account association, which is has_one :account, through: :account_provider, source: :account. includes(:account_provider) only preloads account_provider, not the account reachable through it. Each ma.current_account&.id call triggers an additional query per linked account.

Proposed fix to preload through the association chain

-      account_ids = linked_accounts.includes(:account_provider).filter_map { |ma| ma.current_account&.id }
+      account_ids = linked_accounts.includes(account_provider: :account).filter_map { |ma| ma.current_account&.id }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/syncer.rb` at line 50, The N+1 stems from calling
ma.current_account on each linked_account while only preloading
:account_provider; update the preload to include the through association so
current_account is eager loaded (e.g. change the includes call on
linked_accounts from includes(:account_provider) to include the account via the
provider such as includes(account_provider: :account) or the direct association
name if defined), then keep the filter_map { |ma| ma.current_account&.id }
unchanged so no extra queries are issued.

---

22-31: 💤 Low value

Minor: redundant queries on unlinked_accounts.

unlinked_accounts.any? (line 26) and unlinked_accounts.count (line 28) each fire their own LEFT JOIN query. Since the count is used both for branching and the status text, materializing once would halve the round-trips. Same pattern repeats for linked_accounts.any?/linked_accounts.count on lines 34/37.

Proposed micro-optimization

-    if unlinked_accounts.any?
+    unlinked_count = unlinked_accounts.count
+    if unlinked_count.positive?
       brex_item.update!(pending_account_setup: true)
-      sync.update!(status_text: "#{unlinked_accounts.count} accounts need setup...") if sync.respond_to?(:status_text)
+      sync.update!(status_text: "#{unlinked_count} accounts need setup...") if sync.respond_to?(:status_text)
     else

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/syncer.rb` around lines 22 - 31, Compute and cache the
counts for linked_accounts and unlinked_accounts once instead of calling any?
and count separately: fetch
brex_item.brex_accounts.joins(:account_provider).count into a variable (e.g.,
linked_count) and
brex_item.brex_accounts.left_joins(:account_provider).where(account_providers: {
id: nil }).count into unlinked_count, then use those cached integers for the
branching (if unlinked_count > 0 / else) and for sync.update!(status_text:
"#{unlinked_count} accounts need setup...") as well as for the linked_accounts
logic; update pending_account_setup on brex_item and status_text on sync using
these counts to avoid duplicate DB queries.

---

28-28: ⚡ Quick win

i18n: hardcoded user-facing status strings.

Status text strings like "Importing accounts from Brex...", "Checking account configuration...", "#{unlinked_accounts.count} accounts need setup...", "Processing transactions...", and "Calculating balances..." are hardcoded throughout perform_sync. These are user-visible (rendered in the UI via sync.status_text) and should go through I18n.t to match the localization pattern used elsewhere in the file (e.g., user_safe_error_message already uses I18n.t("brex_items.syncer.*")).

As per coding guidelines: "Always use t() helper for user-facing strings."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/syncer.rb` at line 28, The hardcoded user-facing status
strings in perform_sync (e.g., the arguments passed to sync.update!(status_text:
...)) must be replaced with I18n lookup calls; locate perform_sync in
BrexItem::Syncer and replace each literal like "Importing accounts from
Brex...", "Checking account configuration...", "#{unlinked_accounts.count}
accounts need setup...", "Processing transactions...", and "Calculating
balances..." with t(...) calls (e.g., t("brex_items.syncer.importing_accounts")
etc.), passing dynamic values (like unlinked_accounts.count) as interpolation
locals to I18n and ensure the same translation key namespace used by
user_safe_error_message is used so the UI-texts are localizable.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

app/models/brex_item/account_flow.rb (2)

297-347: ⚡ Quick win

N+1 in complete_setup! — preload brex_accounts with their account_provider association.

The loop issues two queries per submitted account (one find_by + one account_provider.present? association load), totalling 2N queries. Preloading avoids this:

♻️ Proposed refactor

  def complete_setup!(account_types:, account_subtypes:)
    created_accounts = []
    skipped_count = 0
    valid_types = Provider::BrexAdapter.supported_account_types
    failed_count = 0

+   brex_accounts_by_id = brex_item.brex_accounts
+     .includes(:account_provider)
+     .where(id: account_types.keys)
+     .index_by { |a| a.id.to_s }

    account_types.each do |brex_account_id, selected_type|
      ...
-     brex_account = brex_item.brex_accounts.find_by(id: brex_account_id)
+     brex_account = brex_accounts_by_id[brex_account_id.to_s]
      unless brex_account

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 297 - 347, The loop in
complete_setup! is causing an N+1 by calling
brex_item.brex_accounts.find_by(...) and then checking
brex_account.account_provider.present? for each entry; preload the brex_accounts
with their account_provider association once before iterating (e.g., load the
submitted accounts via brex_item.brex_accounts.where(id:
account_types.keys).includes(:account_provider) and index them by id) and then
use that in the loop instead of calling find_by each time so the
account_provider check does not fire extra queries.

---

106-116: ⚡ Quick win

link_new_accounts_result and link_existing_account_result leave StandardError unhandled — inconsistent with selection_result_for.

Both write-path methods rescue only known domain errors. An unexpected ActiveRecord::RecordInvalid (e.g., from Account.create_and_sync or AccountProvider.create!) propagates uncaught through these methods to the controller, resulting in a 500 rather than a user-facing alert. selection_result_for (Line 444) already rescues StandardError—the same defensive pattern should be applied here for consistency.

♻️ Proposed fix

  def link_new_accounts_result(account_ids:, accountable_type:)
    ...
    link_navigation_result(link_new_accounts!(account_ids: account_ids, accountable_type: accountable_type))
  rescue NoApiTokenError
    navigation(:new_account, :alert, I18n.t("brex_items.link_accounts.no_api_token"))
  rescue Provider::Brex::BrexError => e
    navigation(:new_account, :alert, I18n.t("brex_items.link_accounts.api_error", message: e.message))
+ rescue StandardError => e
+   Rails.logger.error("Unexpected error linking Brex accounts: #{e.class}: #{e.message}")
+   navigation(:new_account, :alert, I18n.t("brex_items.errors.unexpected_error"))
  end

  def link_existing_account_result(account:, brex_account_id:)
    ...
  rescue Provider::Brex::BrexError => e
    navigation(:accounts, :alert, I18n.t("brex_items.link_existing_account.api_error", message: e.message))
+ rescue StandardError => e
+   Rails.logger.error("Unexpected error linking existing Brex account: #{e.class}: #{e.message}")
+   navigation(:accounts, :alert, I18n.t("brex_items.errors.unexpected_error"))
  end

Also applies to: 118-136

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/models/brex_item/account_flow.rb` around lines 106 - 116, The methods
link_new_accounts_result and link_existing_account_result currently only rescue
domain errors and let unexpected exceptions (e.g., ActiveRecord::RecordInvalid)
bubble up; add a catch-all rescue StandardError (as selection_result_for does)
to each method so unexpected errors are converted to a user-facing navigation
alert (use the same I18n key/pattern used for API errors and include the
exception message), ensuring any ActiveRecord::RecordInvalid or other
StandardError gets handled and returns a navigation(...) instead of raising to
the controller.

test/controllers/brex_items_controller_test.rb (1)

141-142: ⚡ Quick win

Assert translated flash copy via I18n instead of English literals.

These assertions are checking user-facing copy, so harmless translation or wording updates will fail the suite even when controller behavior is unchanged. Prefer I18n.t(...) in the expectations.

As per coding guidelines "Always use t() helper for user-facing strings."

Also applies to: 268-268, 337-338, 370-370, 383-383, 430-430

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/controllers/brex_items_controller_test.rb` around lines 141 - 142,
Change assertions that compare flash text to hard-coded English to use the I18n
helper instead; e.g. replace assert_equal "Choose a Brex connection in Provider
Settings.", flash[:alert] with assert_equal
t('choose_brex_connection_in_provider_settings', default: "Choose a Brex
connection in Provider Settings."), flash[:alert] (and do the same replacement
for the other occurrences called out). Use t(...) (or I18n.t(...)) with an
appropriate translation key and a default that matches the current English copy
so tests remain stable while allowing future translations.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes